Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renderer flag #37529

Closed
wants to merge 1 commit into from
Closed

Renderer flag #37529

wants to merge 1 commit into from

Conversation

dgp1130
Copy link
Contributor

@dgp1130 dgp1130 commented Jun 10, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
  • [N/A] Tests for the changes have been added (for bug fixes / features)
    • Tested by manually patching this into google3 into a workspace with the new flag and verified that setting it via command line in Blaze correctly enables Ivy as expected.
  • [N/A] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: http://b/158246077

Currently there is no way to set a particular ng_module() to build with View Engine or Ivy via a Bazel transition. Choosing a renderer is an "all or nothing" approach, where the entire compilation uses one renderer or the other.

What is the new behavior?

ng_module() now supports a string_flag() being provided as a _renderer attribute and will use it to decide whether to build with View Engine or Ivy. Such flags can be set via a transition on a per-target basis, making the flag much more flexible for migration use cases.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

This flag is not intended for external use cases and will only ever be set inside google3. It is also not intended to replace --config ivy or --define angular_ivy_enabled=True.

See cl/315749946 for a g3 patch which adds the flag to be compatible with this PR.

@dgp1130 dgp1130 force-pushed the renderer-flag branch 4 times, most recently from e45c89f to bc13db9 Compare June 11, 2020 23:12
This checks for a Bazel flag in `ng_module()` in the `_renderer` attribute
which specifies the renderer to use for the build.

The main advantage of this flag is that it can be overridden with [Bazel
transitions](https://docs.bazel.build/versions/master/skylark/config.html),
giving much more flexibility for migrating individual applications in a
Bazel workspace to Ivy.

This flag is not intended to replace `--config ivy` or
`--define angular_ivy_enabled=True` (although it technically could). As a
result, this flag is not and will not actually be used anywhere in the
`angular/angular` repo. Instead, a `string_flag()` is provided  internally
which sets the renderer via a transition. See http://cl/315749946.

Note that this does **not** introduce a dependency on Skylib for
`angular/angular`. The dependency isn't actually necessary because
`BuildSettingInfo` is not used externally anyways. By doing this, it is not
necessary for downstream, external workspaces to depend on Skylib.
@dgp1130 dgp1130 marked this pull request as ready for review June 11, 2020 23:48
@pullapprove pullapprove bot requested a review from josephperrott June 11, 2020 23:48
@dgp1130 dgp1130 added area: bazel Issues related to the published `@angular/bazel` build rules action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Jun 11, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jun 11, 2020
@dgp1130
Copy link
Contributor Author

dgp1130 commented Jun 11, 2020

/cc @IgorMinar.

@dgp1130 dgp1130 removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 12, 2020
@dgp1130
Copy link
Contributor Author

dgp1130 commented Jun 12, 2020

Forced google3 passing as only failures appear to be flakes (View Engine, Ivy).

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@josephperrott josephperrott removed the request for review from gregmagolan June 12, 2020 03:56
@dgp1130 dgp1130 added the action: merge The PR is ready for merge by the caretaker label Jun 12, 2020
@mhevery mhevery closed this in 1091ddb Jun 12, 2020
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
This checks for a Bazel flag in `ng_module()` in the `_renderer` attribute
which specifies the renderer to use for the build.

The main advantage of this flag is that it can be overridden with [Bazel
transitions](https://docs.bazel.build/versions/master/skylark/config.html),
giving much more flexibility for migrating individual applications in a
Bazel workspace to Ivy.

This flag is not intended to replace `--config ivy` or
`--define angular_ivy_enabled=True` (although it technically could). As a
result, this flag is not and will not actually be used anywhere in the
`angular/angular` repo. Instead, a `string_flag()` is provided  internally
which sets the renderer via a transition. See http://cl/315749946.

Note that this does **not** introduce a dependency on Skylib for
`angular/angular`. The dependency isn't actually necessary because
`BuildSettingInfo` is not used externally anyways. By doing this, it is not
necessary for downstream, external workspaces to depend on Skylib.

PR Close angular#37529
@dgp1130 dgp1130 deleted the renderer-flag branch July 9, 2020 19:09
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 9, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
This checks for a Bazel flag in `ng_module()` in the `_renderer` attribute
which specifies the renderer to use for the build.

The main advantage of this flag is that it can be overridden with [Bazel
transitions](https://docs.bazel.build/versions/master/skylark/config.html),
giving much more flexibility for migrating individual applications in a
Bazel workspace to Ivy.

This flag is not intended to replace `--config ivy` or
`--define angular_ivy_enabled=True` (although it technically could). As a
result, this flag is not and will not actually be used anywhere in the
`angular/angular` repo. Instead, a `string_flag()` is provided  internally
which sets the renderer via a transition. See http://cl/315749946.

Note that this does **not** introduce a dependency on Skylib for
`angular/angular`. The dependency isn't actually necessary because
`BuildSettingInfo` is not used externally anyways. By doing this, it is not
necessary for downstream, external workspaces to depend on Skylib.

PR Close angular#37529
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: bazel Issues related to the published `@angular/bazel` build rules cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants